-
Notifications
You must be signed in to change notification settings - Fork 38
[V4] Sketch of promise structure #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThe pull request reorganizes macro definitions by splitting the public Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@benchmark/src/libfork_benchmark/fib/lf_parts.cpp`:
- Around line 20-21: The thread-local TLS buffer is only byte-aligned causing
tls_stack::operator new to return misaligned memory when sp == buffer; update
the declaration of buffer to use the default-new alignment (e.g.,
alignas(lf::k_new_align)) so coroutine frames get proper alignment (keep sp as
thread_local static std::byte* sp = nullptr and only change the buffer
declaration to include alignas(lf::k_new_align)).
- Around line 30-34: The operator new implementation for tls_stack (the static
operator new(std::size_t) function) advances the stack pointer sp without
checking the 8KB buffer bounds; add a bounds check before updating sp that
ensures sp + align(sz) does not exceed the end of the TLS buffer and, on
overflow, calls std::terminate() to abort safely; also add a top-of-file include
for <exception> (or use import std; in C++20 modules) so std::terminate is
available.
In `@include/libfork/__impl/assume.hpp`:
- Around line 5-8: Update the Doxygen file tag at the top of
include/libfork/__impl/assume.hpp: replace the incorrect "@file compiler.hpp"
tag with "@file assume.hpp" (and adjust the brief description if needed) so the
generated documentation correctly labels this header; locate the tag in the file
header comment block and edit the tag text accordingly.
- Around line 26-30: The NDEBUG branch defines LF_ASSUME as a bare if that can
bind to an outer else; change its expansion to use the same do { ... }
while(false) wrapper as the debug branch so calls to LF_ASSUME(expr) are
statement-safe—i.e., wrap the existing conditional and ::std::unreachable() call
inside a do { if (!(expr)) { ::std::unreachable(); } } while (false) construct
to avoid dangling-else hazards.
In `@include/libfork/__impl/compiler.hpp`:
- Around line 3-10: The documentation in compiler.hpp claims the macros are
"standalone" but the file includes exception.hpp; either remove the `#include`
"libfork/__impl/exception.hpp" from compiler.hpp if the macros truly have no
dependency on exception types, or update the header comment in compiler.hpp to
explicitly state the dependency on libfork/__impl/exception.hpp (and why), so
the documentation and implementation match; locate the include and the
top-of-file comment in compiler.hpp to make the change.
In `@include/libfork/__impl/exception.hpp`:
- Line 69: The LF_RETHROW macro is defined with a typo as LF_TERLF_TERMINATE
which breaks compilation when exceptions are disabled; update the macro
definition for LF_RETHROW to call the correct LF_TERMINATE symbol (i.e., replace
LF_TERLF_TERMINATE with LF_TERMINATE) so the rethrow path uses the intended
terminate macro.
In `@src/core/promise.cxx`:
- Around line 163-167: The template promise_type<T, StackPolicy> is missing
coroutine API methods needed for non-void tasks; add a get_return_object() that
constructs and returns the corresponding task<T,...> (using frame and this
promise as needed) and implement return_value(T value) to store the returned
value into the promise's return_address (ensuring return_address is set by
get_return_object or the task constructor and that you memcpy/assign the value
into *return_address), while preserving existing members frame and StackPolicy;
reference promise_type, get_return_object, return_value, return_address, frame,
and task in your changes.
- Line 179: Fix the typo in the comment header "// =============== std
specialzation =============== //" by renaming "specialzation" to
"specialization" so it reads "// =============== std specialization
=============== //"; locate and update the comment in src/core/promise.cxx
(search for the string "std specialzation") to ensure the spelling is corrected
in the codebase.
🧹 Nitpick comments (1)
src/core/promise.cxx (1)
56-57: Inheriting fromstd::unique_ptris fragile.
std::unique_ptrdoes not have a virtual destructor, so inheriting from it (viaunique_promise) can lead to undefined behavior if ataskis deleted through a base pointer. Consider using composition instead or ensure this type is never used polymorphically.The Cppcheck syntax error is a false positive due to limited C++20 module support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the libfork library's v4 codebase by reorganizing internal headers into a modular structure and introducing core async task infrastructure with frame-based coroutine management.
Changes:
- Reorganized macro definitions from
include/libfork/macros.hppinto modular headers (compiler.hpp,exception.hpp,utils.hpp,assume.hpp) - Added async task infrastructure with coroutine promise types, frame management, and configurable stack allocation policies
- Introduced utility types (
immovable,move_only) and core concepts (returnable,alloc_mixin)
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| include/libfork/macros.hpp | Removed monolithic macro header |
| include/libfork/__impl/compiler.hpp | Extracted compiler-specific attributes (inlining macros) |
| include/libfork/__impl/exception.hpp | Extracted exception handling macros and terminate function |
| include/libfork/__impl/utils.hpp | Extracted utility macros (LF_HOF, LF_FWD) |
| include/libfork/__impl/assume.hpp | Added assertion and assumption macros |
| src/exception.cpp | Implementation of terminate_with function |
| src/core/utility.cxx | Added immovable and move_only mixin types |
| src/core/concepts.cxx | Added core concepts for returnable types and allocation mixins |
| src/core/frame.cxx | Added frame_type for coroutine frame management |
| src/core/constants.cxx | Added alignment and cache line constants |
| src/core/promise.cxx | Complete rewrite with async task/promise infrastructure |
| src/core/core.cxx | Added new module partition exports |
| test/src/utility.cpp | Added tests for utility types |
| benchmark/src/libfork_benchmark/fib/lf_parts.cpp | Added benchmarks for coroutine-based fibonacci |
| benchmark/src/libfork_benchmark/fib/fib.hpp | Updated benchmark parameters |
| benchmark/src/libfork_benchmark/fib/serial.cpp | Updated includes and counter cast |
| benchmark/src/libfork_benchmark/fib/serial_return.cpp | Updated includes and counter cast |
| benchmark/src/libfork_benchmark/common.hpp | Updated include path |
| benchmark/CMakeLists.txt | Enabled new benchmark source |
| CMakeLists.txt | Updated header and module file lists |
| CMakeUserPresets.json | Removed benchmark test preset |
| .clang-tidy | Updated formatting configuration and added ignore rule |
| .clang-format | Added macro definitions for formatting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.